New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Flow: interface identifier should be declared in the scope #10220
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11231/ |
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11160/ |
/cc: @babel/flow |
I've a similar PR #10100, but I've no time to work on it now. Just FYI. |
@tanhauhau Thank you. I should have found from the original issue that you are working on it. As our approaches are pretty similar, Nicolò will try to merge my test case to your implementation. |
Fixes babel#10044
# Conflicts: # packages/babel-plugin-transform-flow-strip-types/test/fixtures/strip-types/def-site-variance/input.js # packages/babel-plugin-transform-flow-strip-types/test/fixtures/strip-types/strip-declare-statements/input.js # packages/babel-plugin-transform-flow-strip-types/test/fixtures/strip-types/strip-interfaces-module-and-script/input.js # packages/babel-plugin-transform-flow-strip-types/test/fixtures/strip-types/strip-iterator/input.js
Update: I have merged #10100 add disabled some test related to Variable[+Declare] and Function[+Declare] @tanhauhau This PR is almost same with #10100 except a61fa14#diff-c3d4a6608f6a39a6a14b0c4f474c922cL197 |
/cc @JLHwung status? need this for prettier |
@fisker we've been waiting for someone on the Flow team to chime in |
@existentialism that's OK, just checking. FYI, this failure test is from flow project, and it has been broken since @bable/parser@7.4.0 |
@existentialism Do you know who on the Flow team should be pinged to move this forward? This is currently the only issue/PR that is blocking Babel upgrade in Prettier. |
Yep, it is blocker for us (prettier) /cc @nmote maybe you can help? |
Mostly if it is being declared in the scope correctly, or if it should follow some other rules (for example regarding to shadowing/redeclaration) |
Unfortunately I'm not really familiar with that area of the parser. My process would basically be to look at how it behaves in practice, which @JLHwung has already done. |
Well, let's merge this! |
@nicolo-ribaudo any ETA for patch release? |
A follow up to #9493: Declare interface identifier in the scope so that it would not be an undefined export. Thanks @jj-choi for suggestions to the solution.
As I didn't find any specification for Flow, I have developed several test cases on Flow REPL. We revise the
flowParseInterfaceish
according to the following observations:LEXICAL
interface I {}; interface I {};
will throwLEXICAL
type A = string; type A = string;
will throwLEXICAL
opaque type A = string; opaque type A = string'
will throwVAR
declare class A {}; declare class A{};
does not throwbut
declare class A {}; class A{};
will throwdeclare var A; declare var A;
does not throwdeclare var A; declare var A; var A
does not throwdeclare function A(): void; declare function A(): void; function A() {}
does not throwSince Class[+Declare] implies a
VAR
type binding identifier, I have changed the scope type ofDeclareModule
toSCOPE_VAR
, otherwise the following snippet will throw:Known issues
We haven't applied
declareName
for Variable[+Declare] and Function[+Declare] since they can share the identifier name withVariableDeclaration
andFunctionDeclaration
, which means the following snippet would failWe may solve this issue by introducing
flowVars
toScope
and record them in an array other thanscope.vars
. However, I suggest we submit another PR to solve it before this one gets too big to review.